Thsi went away now in favor of just using PackageParseError

Note - I named it "package parse", because in the future we might want to expose a dedicated error for the "file parse" stage (a single WIT package can consist of many files).

Parsing is also usually thought of as converting a single file into an AST, so I think mentioning that this is about parsing a whole package makes things a bit clearer.

Note that we're now returning the source map as well. Otherwise the consumers would need to clone the source map if they want to use it for resolving spans (since parse consumes self).

Do you think it'd be reasonable to change this signature to &self to avoid the need for that? That's a bit more idiomatic and I'm not seeing off the top of my head why self is used here vs &self

The reason it takes self is that UnresolvedPackageGroup takes the ownership of source_map: SourceMap, so we'd either have to clone or introduce a lifetime.

SourceMap contains the contents of multiple files so it might not be very cheap to clone.

I will investigate if UnresolvedPackageGroup can just contain a reference to a source map.

This will go away once the resolver is also migrated to structured errors.

Previously we were doing a mix of "does not exist" and "is not defined". I'm going to standarize on "does not exist".

Once the structured error migration is done we might want to audit all error messages and see if anything could be better (now that we can track rich metadata easily).

Bikeshedding this slightly, one option here might be:

return Err(PackageParseErrors::new_syntax(f.name.span, "duplicate constructors"))

where the constructor new_syntax takes message: impl Into<String> or somthing like that.

Would you want me to do this for all variants? I'm worried about things like:

    ItemNotFound {
        span: Span,
        name: String,
        kind: String,
        hint: Option<String>,
    },

where the order of strings matters (ie PackageParseErrors::item_not_found(span, "A", "B", None) is different to PackageParseErrors::item_not_found(span, "B", "A", None)) and it's easy to accidentally mix it up.

And we'll also have some variants that contain multiple spans (e.g. duplicate package).

s/PackageParseErrorKind/PackageParseErrors/

Do you think it'd be reasonable to change this signature to &self to avoid the need for that? That's a bit more idiomatic and I'm not seeing off the top of my head why self is used here vs &self

We'll generally want to avoid stringifying errors like this since it loses type information -- but isn't the highlighting handled by by parse itself?

but isn't the highlighting handled by by parse itself?

There are two parses here - SourceMap.parse and UnresolvedPackageGroup.parse. The former is what we call here, and it doesn't do highlights anymore (it returns structured errors which can then be highlighted when needed).

We'll generally want to avoid stringifying errors like this since it loses type information

Correct. I just didn't touch the UnresolvedPackageGroup struct for now. This struct just contains methods that are a wrapper for things like "read file, add string to SourceMap, call SourceMap.parse", ie:

• UnresolvedPackageGroup::parse - convert path to string, then parse the contents
• UnresolvedPackageGroup::parse_str - this is a thin wrapper around SourceMap::parse so it can start returning PackageParseErrors
• UnresolvedPackageGroup::parse_path - calls UnresolvedPackageGroup::parse_dir or UnresolvedPackageGroup::parse_file depending on whether the path is a dir or a file
• UnresolvedPackageGroup::parse_file - reads the file contents, and then parses them
• UnresolvedPackageGroup::parse_dir - adds all files in the directory to the source map, then parses it.

If we were to move all of these to return ParseErrors, then ParseErrors` would have to gain variants for bad IO/path operations, and I'm not sure if we want to go there? It might be good to keep parsing totally separate from things like this.

I actually considered deprecating/removing most of these methods, because the consumers can just replicate the functionality themselves. What do you think?

Mind adding some brief comments to public types/enums/structs in this file as well as the module itself?

To bikeshed a bit, I find the naming here a bit too wordy. WDYT about something like this:

• PackageParseErrors => ParseErrors.
• (future) FileParseErrors => FileErrors
• (new) type ParseResult<T, E = ParseErrors> = Result<T, E>
• (new) type LexResult<T, E = lex::Error> = Result<T, E>

That to me keeps the same flexibility for package/file distinctions insofar as there's different classes of errors, but I think it's useful for error ergonomics to not really get in the way when reading/writing code. To that extent I'm worried how verbose the error handling here is, so I'm hoping some renamings/idioms can help reduce the noise without adding too much complexity.

Perfect. I'll do that.